upstream: partition hosts in a single pass#6440
Conversation
This fixes a performance regression that was introduced when support for degraded hosts was added: the list of hosts would be iterated over four times instead of the previous two (one for the hosts list, one for the hosts per locality list). This PR changes both partition operations to only iterate over the list of hosts once. Signed-off-by: Snow Pettersen <snowp@squareup.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, looks good. A few small comments for thought.
/wait
| HostsPerLocalityImpl::filter(std::function<bool(const Host&)> predicate) const { | ||
| auto* filtered_clone = new HostsPerLocalityImpl(); | ||
| HostsPerLocalityConstSharedPtr shared_filtered_clone{filtered_clone}; | ||
| return filter(std::vector<std::function<bool(const Host&)>>{predicate})[0]; |
There was a problem hiding this comment.
Can we just get rid of this variant and have callers use the vector version? It doesn't seem worth it to me to maintain both, but I don't feel strongly about it if you do. WDYT?
There was a problem hiding this comment.
Yeah i'll just remove this
| // We keep two lists: one for being able to mutate the clone and one for returning to the caller. | ||
| // Creating them both at the start avoids iterating over the mutable values at the end to convert | ||
| // them to a const pointer. | ||
| std::vector<HostsPerLocalityImpl*> mutable_clones; |
There was a problem hiding this comment.
It's a little odd that we are using a raw pointer for this vector. Can we use a shared_ptr here also? I think a mutable shared pointer can be shared ptr casted to const so this should work but not 100% sure.
| Host::Health); | ||
| // Partitions the provided list of hosts into two new lists containing the healthy and degraded | ||
| // hosts respectively. | ||
| static std::pair<HostVectorConstSharedPtr, HostVectorConstSharedPtr> |
There was a problem hiding this comment.
Is it worth doing the same type tricks you did previously to make it more clear that the first is healthy and the second is degraded? I think this might be worth it in the this case?
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
stale
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Awesome, I would merge master to pick up flake fixes.
Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
/retest |
|
🙀 Error while processing event: |
2 participants
This fixes a performance regression that was introduced when support for
degraded hosts was added: the list of hosts would be iterated over four
times instead of the previous two (one for the hosts list, one for the
hosts per locality list). This PR changes both partition operations to
only iterate over the list of hosts once.
Signed-off-by: Snow Pettersen snowp@squareup.com
Risk Level: Low
Testing: Existing UTs
Docs Changes: n/a
Release Notes: n/a